Conversation
Generated by 🚫 Danger |
SimplyDanny
left a comment
There was a problem hiding this comment.
Nice one! Glad to see that DeclaredIdentifiersTrackingVisitor is used for more advanced rules.
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
| print(outer) | ||
| } | ||
| """), | ||
| ] |
There was a problem hiding this comment.
There should be a few more example involving properties, local variables, parameters, functions, tuples, optional bindings, ...
There was a problem hiding this comment.
Examples added for all the use cases
There was a problem hiding this comment.
What about:
var a = 1
if let a = b {}var i = 1
for i in list {}struct S {
var a = 1
var b: Int {
let a = 2
return a
}
}var a = 1
func a(a: Int) {} // Should we trigger for the function name and/or the parameter (optionally)?There was a problem hiding this comment.
Thanks for highlighting, i have added additional triggering examples, expanding the triggering examples
There was a problem hiding this comment.
var a = 1
func a(a: Int) {} // Should we trigger for the function name and/or the parameter (optionally)?
Triggering for parameters is optional, but the current behavior (not triggering) is standard and user-friendly. If you want to add an option to control this, I can add a configuration flag (e.g., ignoreParameters: true/false).
There was a problem hiding this comment.
Thanks for highlighting, i have added additional triggering examples, expanding the triggering examples
Sure? I can't find them.
Triggering for parameters is optional, but the current behavior (not triggering) is standard and user-friendly. If you want to add an option to control this, I can add a configuration flag (e.g., ignoreParameters: true/false).
I'd prefer an option for that as I think some would like that. However, I leave it open to you to implement it or ignore it for now. In the latter case, this can be a follow-up.
There was a problem hiding this comment.
Thanks for highlighting, i have added additional triggering examples, expanding the triggering examples
Sure? I can't find them.
Apologies, didn't push it yet.
Triggering for parameters is optional, but the current behavior (not triggering) is standard and user-friendly. If you want to add an option to control this, I can add a configuration flag (e.g., ignoreParameters: true/false).
I'd prefer an option for that as I think some would like that. However, I leave it open to you to implement it or ignore it for now. In the latter case, this can be a follow-up.
Yes, i can look into this, will tag you once all the changes are pushed
There was a problem hiding this comment.
Hi @SimplyDanny, I have updated the rules with configuration and also added relevent examples, so all changes are pushed now
There was a problem hiding this comment.
I'm still missing
struct S {
var a = 1
var b: Int {
let ↓a = 2
return a
}
}And like if expressions, we can also have while let a = ... {}
and guard let a = ... else {}.
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
Source/SwiftLintBuiltInRules/Rules/Lint/VariableShadowingRule.swift
Outdated
Show resolved
Hide resolved
| print(outer) | ||
| } | ||
| """), | ||
| ] |
There was a problem hiding this comment.
What about:
var a = 1
if let a = b {}var i = 1
for i in list {}struct S {
var a = 1
var b: Int {
let a = 2
return a
}
}var a = 1
func a(a: Int) {} // Should we trigger for the function name and/or the parameter (optionally)?3553d5d to
eb602f2
Compare
| severity: warning | ||
| ignore_parameters: true | ||
| meta: | ||
| opt-in: false |
There was a problem hiding this comment.
Rule should be opt-in.
| print(outer) | ||
| } | ||
| """), | ||
| ] |
There was a problem hiding this comment.
I'm still missing
struct S {
var a = 1
var b: Int {
let ↓a = 2
return a
}
}And like if expressions, we can also have while let a = ... {}
and guard let a = ... else {}.
| override func visit(_ node: VariableDeclSyntax) -> SyntaxVisitorContinueKind { | ||
| if node.parent?.is(MemberBlockItemSyntax.self) == false { | ||
| node.bindings.forEach { binding in | ||
| checkForShadowing(in: binding.pattern) |
There was a problem hiding this comment.
No test case fails if checkForBindingShadowing is used here. Is checkForShadowing really necessary? I actually doubt it. If it is, we need an example.
| import TestHelpers | ||
| import XCTest | ||
|
|
||
| final class VariableShadowingRuleTests: SwiftLintTestCase { |
There was a problem hiding this comment.
This test isn't needed. You already check the option in the rule's examples.
|
|
||
| /// Checks all scope levels including the current one. Used for parameter checking | ||
| /// since parameters are declared into a child scope, not the current one. | ||
| private func isShadowingAnyScope(_ identifier: String) -> Bool { |
There was a problem hiding this comment.
We might not need this anymore once #6589 is merged.
…swift Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
…swift Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
…swift Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
…swift Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
Co-authored-by: Danny Mösch <danny.moesch@icloud.com>
5bcb7bf to
fd51bd2
Compare
Summary
Adds a new
variable_shadowingrule that detects when a variable declaration shadows an identifier from an outer scope.Changes
VariableShadowingRule.swift- flags variable declarations that shadow outer scope identifiersBuiltInRules.swiftGeneratedTests_10.swiftdefault_rule_configurations.ymlCHANGELOG.mdExample
Resolves #6228